Skip to content

Move multiproof hashes allocation under condition to avoid zero-length alloc #6092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Move multiproof hashes allocation under condition to avoid zero-length alloc #6092

wants to merge 2 commits into from
+90 −95

Conversation

@Snezhkko
Copy link
Contributor

@Snezhkko Snezhkko commented Nov 10, 2025

This change relocates the allocation of the temporary hashes array and related counters inside the if (proofFlagsLen > 0) branch across all four multiproof functions in MerkleProof.sol. When proofFlagsLen == 0, the function returns early and hashes is never used; previously this still incurred an unnecessary zero-length memory allocation and variable initialization. The refactor avoids that overhead without changing semantics, consistent with existing docs and tests.

@Snezhkko Snezhkko requested a review from a team as a code owner November 10, 2025 20:07
@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: ac06754

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 10, 2025

Walkthrough

Code blocks handling hashes array initialization and the main per-proof loop processing in MerkleProof functions have been refactored to execute conditionally. These operations are now placed inside the if (proofFlagsLen > 0) guards, restricting their execution to when the proof flags length is greater than zero. Previously, these allocations and iterations were unconditionally executed. The surrounding control flow logic, post-loop proof length validation, and conditional return statements remain unchanged. No modifications to public function signatures were introduced.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: moving the hashes allocation under a conditional to avoid zero-length allocation in multiproof functions.
Description check ✅ Passed The description clearly explains the change, the reason for it, and confirms that semantics are preserved, which directly relates to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6308fdc and e1afcbe.

📒 Files selected for processing (1)
  • contracts/utils/cryptography/MerkleProof.sol (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: Pages changed - solidity-contracts
  • GitHub Check: coverage
  • GitHub Check: halmos
  • GitHub Check: tests-foundry
  • GitHub Check: tests-upgradeable
  • GitHub Check: slither
  • GitHub Check: tests
🔇 Additional comments (4)
contracts/utils/cryptography/MerkleProof.sol (4)

223-238: LGTM! Clean optimization that avoids zero-length allocation.

Moving the hashes array allocation and loop variables inside the if (proofFlagsLen > 0) guard is correct and improves gas efficiency. When proofFlagsLen == 0, the loop never executed anyway, so this change simply avoids the unnecessary zero-length memory allocation and variable initialization.

310-325: LGTM! Consistent optimization applied.

The same optimization pattern correctly applied to the custom hashing variant. The refactor maintains semantic equivalence while avoiding unnecessary allocations.

395-410: LGTM! Optimization correctly applied to calldata variant.

The refactor correctly extends the optimization to the calldata variant, maintaining consistency across all multiproof function variants.

482-497: LGTM! Complete and consistent optimization across all variants.

The final multiproof function variant correctly implements the same optimization. All four multiproof functions now consistently avoid unnecessary allocations when proofFlagsLen == 0.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ernestognw
ernestognw approved these changes Nov 13, 2025
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. We'll need a second review here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ernestognw ernestognw ernestognw approved these changes

Assignees

No one assigned

Labels

ignore-changeset

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Snezhkko @ernestognw